-
Notifications
You must be signed in to change notification settings - Fork 43
Add MIPS extensions #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @asb :) |
|
This looks good to me, and uncontroversial :) |
|
Thanks for the PR! I see that the references to the two new extensions go to the product description of the MIPS P8700 Series. Also, the docs on the product page don't mention the two extensions (Xmipscmov and Xmipslsp). The appendix of the P8700 programmer's guide only lists nine custom instructions (ccmov, ehb, ihb, ldp, lwp, mtlbwr, pref, sdp, swp). This repo is not the right place to define extensions. Instead, a document should be referenced where the extensions are defined (incl. CSRs, instructions, etc). A specification of a custom extension should also include the vendor prefix (e.g. |
@cmuellner thanks for the comment! Sorry for the delay here. We have updated the document at [0], and in the |
2fbe34c to
522af6b
Compare
|
Do you have any comment on this? |
|
LGTM, @cmuellner any final comments? |
|
Ping. :) Any comments? |
|
Ping. :) |
kito-cheng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as a RISC-V GCC maintainer
|
The links are already out-of-date. On the referenced page are "Product Brief", "Programmer's Guide", and "Data Sheet". However, the PR states "MIPS p8700 specification", which is not on the linked page. Another thing that I noticed is that the PR introduces an extension with the name "xmipscmove," but the document names it "xmipscmov" (without the e at the end). If these two items are addressed, we can land this. |
522af6b to
bfe0ce1
Compare
|
@cmuellner thanks for the comment. I have addressed it and updated the links. Thank you. |
Adding extensions for MIPS vendor: 1. xmipscmov 2. xmipslsp 3. xmipscbop 4. xmipsexectl The official product page here: https://mips.com/products/hardware/p8700/
bfe0ce1 to
26d700b
Compare
cmuellner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adding extensions for MIPS vendor:
1. xmipscmov
2. xmipslsp
3. xmipscbop
4. xmipsexectl
The official product page here:
https://mips.com/products/hardware/p8700/
Upstreaming of this in LLVM and binutils is in progress.